What the gap is

This PR adds @modelcontextprotocol/codemod as a new published package — it has a bin entry (mcp-codemod), a programmatic API export, and is added to .github/workflows/publish.yml:43 so pkg-pr-new is already publishing preview builds. But packages/codemod/ contains no README.md. Every other published package in the repo has one: packages/client/README.md, packages/server/README.md, and all four packages/middleware/*/README.md files exist.

The repo's own review checklist (REVIEW.md) explicitly calls this out: "New feature: verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example." A new package with both a CLI and a programmatic API is squarely in scope for that item.

Why it matters

The PR description already contains exactly the documentation users need — the CLI Usage block (mcp-codemod v1-to-v2 ./src, --dry-run, --transforms, --list, --ignore), the Programmatic API snippet (getMigration / run), and the 9-row transform table. None of that will be visible on npm. A user who runs npx @modelcontextprotocol/codemod and gets the commander help text will have nowhere to look for the transform IDs, the design decisions, or the migration scope.

Step-by-step proof

1. .github/workflows/publish.yml:43 adds './packages/codemod' to the pkg-pr-new publish command, so the package is published on every PR push (and presumably on release).
2. packages/codemod/package.json:32-34 sets "files": ["dist"] and a bin entry — so the package is intended for end-user consumption via npx.
3. ls packages/codemod/ shows: eslint.config.mjs, package.json, src/, test/, tsconfig.json, tsdown.config.ts, typedoc.json, vitest.config.js — no README.md.
4. ls packages/*/README.md packages/middleware/*/README.md shows READMEs for client, server, express, fastify, hono, and node — codemod is the only published package without one.
5. On npm, the package page renders the README as the primary documentation surface. With no README, the page shows only the package.json description string: "Codemod to migrate MCP TypeScript SDK code from v1 to v2" — no usage, no flags, no transform list.

Note on files array

npm always includes README* (along with package.json, LICENSE*, and the main entry) regardless of the files allowlist, so adding packages/codemod/README.md is sufficient — no need to touch "files": ["dist"].

Fix

Add packages/codemod/README.md containing (at minimum) the CLI Usage block, the Programmatic API snippet, and the transform table from this PR's description. Given the PR is explicitly titled "draft" and the author has an open TODO checklist, this is likely already planned — flagging it here per the REVIEW.md checklist item so it doesn't slip.

What the bug is

isImportedFromMcp(sourceFile, symbolName) (importUtils.ts:76-81) tests imp.getNamedImports().some(n => n.getName() === symbolName). ts-morph's ImportSpecifier.getName() returns the export name (the identifier before as), not the local alias — this is the same fact already established and acted on elsewhere in this PR (e.g., resolved comment 3132456083, and removedApis.ts using foundImport.getAliasNode()?.getText() ?? 'IsomorphicHeaders' to derive the local name).

Two of the four callers pass the local identifier text as it appears in the body:

• handlerRegistration.ts:40 — schemaName = firstArg.getText()
• schemaParamRemoval.ts:31 — schemaName = secondArg.getText()

For these callers, comparing the body identifier against the export name is a category error whenever an alias is involved.

The concrete manifestation: a test that asserts wrong behavior

The most actionable evidence is the new test at handlerRegistration.test.ts:108-119:

import { CallToolRequestSchema } from './local-schemas.js';
import { CallToolRequestSchema as McpSchema } from '@modelcontextprotocol/sdk/types.js';
server.setRequestHandler(CallToolRequestSchema, async () => ({ content: [] }));
validateSchema(McpSchema);

In this fixture, the CallToolRequestSchema passed to setRequestHandler is the local binding from ./local-schemas.js — the MCP one is aliased to McpSchema. The test asserts expect(result).toContain("'tools/call'") and not.toMatch(/setRequestHandler\(CallToolRequestSchema/), i.e., it expects the local schema usage to be rewritten as if it were the MCP schema. That's the false positive, encoded as expected behavior. (The test was presumably written to verify that removeUnusedImport(..., onlyMcpImports=true) doesn't strip the local import — which it does verify — but the rewrite assertion is incorrect.)

Step-by-step proof (false positive)

1. firstArg.getText() = 'CallToolRequestSchema'.
2. ALL_SCHEMA_TO_METHOD['CallToolRequestSchema'] = 'tools/call' → passes line 38.
3. isImportedFromMcp(sf, 'CallToolRequestSchema'): iterates imports; the second declaration is an MCP specifier; its named import has getName() === 'CallToolRequestSchema' (the export name on the aliased specifier) → returns true.
4. firstArg.replaceWithText("'tools/call'") — but firstArg referred to the local ./local-schemas.js binding, not the MCP one.
5. removeUnusedImport(sf, 'CallToolRequestSchema', true) then scans for body references to CallToolRequestSchema, finds none (it was just replaced), and — correctly — only removes the MCP-aliased specifier? Actually no: removeUnusedImport at importUtils.ts:96 also matches on namedImport.getName() (export name), so it would try to remove the aliased MCP specifier, but McpSchema is still referenced in validateSchema(McpSchema), so referenceCount > 0 and nothing is removed. Hence the local import survives (which is what the test's first assertion checks), but the body call was still wrongly rewritten.

Addressing the refutation

A reviewer objected that (a) the proposed fix breaks the other two callers, (b) the false-negative paths are gated elsewhere, and (c) the trigger is contrived. All three points are fair and worth incorporating:

• (a) is correct and important. mcpServerApi.ts:15 and expressMiddleware.ts:12 pass literal export names ('McpServer', 'hostHeaderValidation') as a file-level gate. For import { McpServer as Server }, n.getName() === 'McpServer' is exactly what they want. Changing the function to only check the alias would regress them. So the fix is not the one-liner originally proposed; it's either a second helper (e.g., isLocalIdentifierFromMcp) for the body-identifier callers, or having the function match either form and letting callers 1/2 pass the local text while callers 3/4 keep passing the export name — but "match either" doesn't fix the false positive, so a separate helper is cleaner.
• (b) is largely correct. The false-negative in handlerRegistration is moot because ALL_SCHEMA_TO_METHOD[aliasName] is undefined and short-circuits first. In schemaParamRemoval, isSchemaReference requires the text to end in 'Schema', so an alias like RS is filtered out before the provenance check; only an alias that itself ends in Schema (e.g., as ResultSchema) reaches the bug and is then silently skipped — safe (no corruption), just an incomplete migration.
• (c) is correct on frequency, which is why this is a nit. But the fact that the test suite itself encodes the false positive as expected output is worth a one-line correction regardless of how rare the pattern is in the wild — tests that assert wrong behavior tend to bite later.

Impact

Nit severity. The output-corrupting path requires importing the same export name from both an MCP package (aliased) and a non-MCP source in one file — uncommon. The false-negative paths are mostly gated by earlier checks and, where reachable, leave code unchanged. The main value of the fix is (1) correcting the test so it doesn't lock in a wrong rewrite, and (2) eliminating an internal inconsistency where one helper serves two semantically different caller intents.

Fix

Introduce a local-binding variant for the body-identifier callers and leave the existing function for the file-level gates:

export function isLocalIdentifierFromMcp(sourceFile: SourceFile, localName: string): boolean {
    return sourceFile.getImportDeclarations().some(imp => {
        if (!isAnyMcpSpecifier(imp.getModuleSpecifierValue())) return false;
        return imp.getNamedImports().some(n => (n.getAliasNode()?.getText() ?? n.getName()) === localName);
    });
}

Use it at handlerRegistration.ts:40 and schemaParamRemoval.ts:31. Then update the test at handlerRegistration.test.ts:108-119 to assert the correct behavior: the setRequestHandler(CallToolRequestSchema, ...) call (local binding) is not rewritten, and 'tools/call' does not appear.

What the bug is

The ExportSpecifier branch at astUtils.ts:10-14 was added in response to comment 3136237226 to rewrite export { McpError } → export { ProtocolError as McpError }. But it only checks Node.isExportSpecifier(parent) and then unconditionally operates on parent.getNameNode() — it never verifies whether the matched identifier node is the name node (the local binding, before as) or the alias node (the public export name, after as). Every other guard in this function (lines 15-24: PropertyAssignment, PropertyAccessExpression, PropertySignature, MethodDeclaration, BindingElement, etc.) explicitly checks parent.getNameNode() === node before acting; this branch is the inconsistent one.

Code path that triggers it

In ts-morph, for export { Foo as McpError }, ExportSpecifier.getNameNode() returns the Foo identifier and getAliasNode() returns the McpError identifier. forEachDescendant visits both identifiers. When it reaches the alias McpError:

1. Node.isIdentifier(node) && node.getText() === 'McpError' → true (matches oldName).
2. Node.isExportSpecifier(parent) → true.
3. parent.getAliasNode() is truthy (it's the McpError node) → setAlias is skipped.
4. parent.getNameNode().replaceWithText('ProtocolError') → replaces Foo with ProtocolError.

Result: export { ProtocolError as McpError }. The user's local Foo binding is silently dropped, and ProtocolError may not even exist as a local binding (if the file imported the unaliased MCP McpError, the import was renamed to ProtocolError, so it happens to resolve — but the semantics changed: the module now exports the MCP error class instead of the user's Foo).

Why existing code doesn't prevent it

The branch correctly handles two of three cases: (a) export { McpError } — no alias, setAlias(oldName) then rename → export { ProtocolError as McpError } ✓; (b) export { McpError as Something } — forEachDescendant matches the name node McpError, alias is truthy so setAlias skipped, name replaced → export { ProtocolError as Something } ✓. But case (c) export { Foo as McpError } — where only the alias matches oldName — falls through the same code path and corrupts the name slot. The new test 'preserves export specifier public name with alias' (symbolRenames.test.ts) only covers case (a). No test covers an export specifier whose alias matches a SIMPLE_RENAMES key.

Step-by-step proof

Input (file imports MCP McpError to trigger renameAllReferences, and separately re-exports a local class under the same public name):

import { McpError } from '@modelcontextprotocol/sdk/types.js';
class AppError extends Error {}
export { AppError as McpError };
if (e instanceof McpError) { ... }

1. symbolRenamesTransform finds McpError in the MCP import, calls namedImport.setName('ProtocolError'), then renameAllReferences(sf, 'McpError', 'ProtocolError').
2. forEachDescendant visits the import specifier → Node.isImportSpecifier(parent) returns early. ✓
3. Visits McpError in the instanceof check → falls through to node.replaceWithText('ProtocolError'). ✓
4. Visits the alias McpError in export { AppError as McpError } → parent is ExportSpecifier, getAliasNode() truthy → getNameNode().replaceWithText('ProtocolError') rewrites AppError.

Output:

import { ProtocolError } from '@modelcontextprotocol/sdk/types.js';
class AppError extends Error {}
export { ProtocolError as McpError };   // ← was AppError; now exports the MCP class instead
if (e instanceof ProtocolError) { ... }

The module's public McpError export silently changed from the user's AppError to the SDK's ProtocolError — still compiles, no diagnostic, semantically wrong. (One verifier reproduced this empirically against ts-morph.)

Impact

Nit severity: the trigger requires a file that both (a) imports a SIMPLE_RENAMES key unaliased from MCP (to fire renameAllReferences) and (b) re-exports a different local under that same public name — uncommon but valid TypeScript. When it fires, though, it's the worst codemod failure mode: silent semantic corruption that type-checks. And it's a one-line internal-consistency fix in a branch that was just added in response to review.

Fix

Add a guard at the top of the branch matching the pattern used by every sibling:

if (Node.isExportSpecifier(parent)) {
    if (parent.getAliasNode() === node) return;   // alias is the public export name — leave it
    if (!parent.getAliasNode()) parent.setAlias(oldName);
    parent.getNameNode().replaceWithText(newName);
    return;
}

(Equivalently, if (parent.getNameNode() !== node) return;.) A test like import { McpError } from '@modelcontextprotocol/server'; class Foo {} export { Foo as McpError }; → expect output to contain export { Foo as McpError } would lock this in.

What the bug is

isImportedFromMcp (importUtils.ts:76-84) was changed in the latest commit from n.getName() === symbolName (matching the export name) to:

const localName = n.getAliasNode()?.getText() ?? n.getName();
return localName === symbolName;

i.e., it now matches only the local binding name. This was done to fix the false-positive flagged in comment 3137901202 for handlerRegistration.ts:40 and schemaParamRemoval.ts:31, which pass body-identifier text (a local name). But that same comment explicitly warned: "mcpServerApi.ts:15 and expressMiddleware.ts:12 pass hardcoded export names and rely on the current semantics, so the fix is to check the local binding only for the body-identifier callers — e.g., a separate helper or an optional flag — rather than changing the function unconditionally." The function was changed unconditionally anyway, and those two callers were not updated.

Code path that triggers it

mcpServerApi.ts:15 calls isImportedFromMcp(sourceFile, 'McpServer') as a file-level gate; expressMiddleware.ts:12 calls isImportedFromMcp(sourceFile, 'hostHeaderValidation'). Both pass the export name as a literal string. With an aliased import, ts-morph's getAliasNode()?.getText() returns the alias, so localName is the alias — which never equals the hardcoded export name.

Why this is a regression

Before the latest commit, for import { McpServer as Server } from '@modelcontextprotocol/sdk/server/mcp.js', n.getName() === 'McpServer' was true, the gate passed, and the collection loop (which has no receiver check) correctly migrated server.tool(...) → server.registerTool(...). After the change, localName = 'Server', 'Server' !== 'McpServer', the gate returns false, and the transform returns { changesCount: 0 } without touching any .tool()/.prompt()/.resource() calls. No diagnostic is emitted. No test in mcpServerApi.test.ts covers an aliased McpServer import, so the regression is uncaught. (For expressMiddleware, the gate has the same issue, though the body check at line 32 was already literal-name-based — so the net effect there is unchanged; the observable regression is in mcpServerApi.)

Step-by-step proof

Input:

import { McpServer as Server } from '@modelcontextprotocol/sdk/server/mcp.js';
const s = new Server({ name: 'test', version: '1.0' });
s.tool('greet', async () => ({ content: [] }));

1. mcpServerApiTransform.apply() runs; line 15 calls isImportedFromMcp(sf, 'McpServer').
2. importUtils.ts:78 — the import's specifier passes isAnyMcpSpecifier.
3. importUtils.ts:80 — the single named import has getAliasNode()?.getText() = 'Server', so localName = 'Server'.
4. importUtils.ts:81 — 'Server' === 'McpServer' → false. .some() returns false.
5. mcpServerApi.ts:15-17 — gate fails → return { changesCount: 0, diagnostics: [] }.
6. s.tool('greet', ...) is never migrated. After importPaths rewrites the import to @modelcontextprotocol/server, the user is left with a v2 import and an un-migrated v1 .tool() call that produces a TypeScript error — with zero codemod diagnostics pointing at it.

Impact

Aliasing McpServer (e.g., as Server to avoid the Mcp prefix, or to disambiguate from another Server class) is uncommon but realistic. The failure mode is safe — the source is left unchanged rather than corrupted, and TypeScript will flag the unmigrated .tool() calls — hence nit severity. But it's a regression from the immediately-preceding commit, it was explicitly predicted in the still-unresolved review comment, and the fix is small.

Fix

Do not change isImportedFromMcp to match either export-or-local name — that would re-introduce the handlerRegistration false positive (and break the new test 'does not rewrite local import when aliased MCP import has same export name'). Instead, give the file-level gates an export-name check:

// importUtils.ts
export function isMcpExportImported(sourceFile: SourceFile, exportName: string): boolean {
    return sourceFile.getImportDeclarations().some(imp =>
        isAnyMcpSpecifier(imp.getModuleSpecifierValue()) &&
        imp.getNamedImports().some(n => n.getName() === exportName)
    );
}

Use it at mcpServerApi.ts:15 and expressMiddleware.ts:12. (Or add a { matchExportName?: boolean } flag to the existing helper.) A test with import { McpServer as Server } + s.tool(...) → expect registerTool would lock this in.

What the bug is

The newly-added symbolTargetOverrides mechanism lets a single v1 module map different symbols to different v2 packages — currently used only for @modelcontextprotocol/sdk/server/streamableHttp.js, where StreamableHTTPServerTransport goes to @modelcontextprotocol/node while everything else (EventStore, StreamId, etc.) goes to @modelcontextprotocol/server. The non-aliased path at importPaths.ts:138-144 handles this correctly by routing each specifier individually:

const symbolTarget = mapping.symbolTargetOverrides?.[name] ?? targetPackage;
addPending(symbolTarget, [resolvedName], specifierTypeOnly);

But line 106 computes hasAlias = namedImports.some(n => n.getAliasNode() !== undefined), and if any specifier is aliased the whole declaration is sent into the in-place setModuleSpecifier branch (107-136). That branch can only set one module specifier, and the allOverridden guard at line 110 only redirects to the override package when every named import is a key in symbolTargetOverrides. For a mixed import, allOverridden is false, effectiveTarget stays at the base targetPackage (@modelcontextprotocol/server), and the renamed transport ends up imported from the wrong package.

Code path that triggers it

Input:

import { StreamableHTTPServerTransport as T, EventStore } from '@modelcontextprotocol/sdk/server/streamableHttp.js';

(Note that aliasing just one of the two specifiers is enough — .some() at line 106 fires on a single match.)

1. mapping = { target: '@modelcontextprotocol/server', renamedSymbols: { StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' }, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }.
2. Lines 100-104: renameAllReferences(sourceFile, 'StreamableHTTPServerTransport', 'NodeStreamableHTTPServerTransport') runs (no-op here since the body uses the alias T).
3. Line 106: hasAlias = true → enters the in-place branch.
4. Line 110: namedImports.every(n => n.getName() in {StreamableHTTPServerTransport: ...}) — EventStore is not a key → allOverridden = false.
5. effectiveTarget stays '@modelcontextprotocol/server'; line 115 imp.setModuleSpecifier('@modelcontextprotocol/server').
6. Lines 117-121: the specifier StreamableHTTPServerTransport is renamed to NodeStreamableHTTPServerTransport (alias T preserved).

Output:

import { NodeStreamableHTTPServerTransport as T, EventStore } from '@modelcontextprotocol/server';

→ TS2305: Module '"@modelcontextprotocol/server"' has no exported member 'NodeStreamableHTTPServerTransport'. No diagnostic is emitted (the only warning(...) in this branch is gated on namespaceImport).

Why existing code/tests don't catch it

The two existing tests bracket this case without covering it: 'handles aliased renamedSymbols correctly' (importPaths.test.ts:165) tests a single aliased specifier (so allOverridden is true → routes to /node ✓), and 'splits streamableHttp import: transport to /node, types to /server' (importPaths.test.ts:177) tests a mixed but unaliased import (so hasAlias is false → per-symbol addPending path ✓). The aliased-and-mixed combination falls between them.

The same all-or-nothing logic appears in rewriteExportDeclarations (line 222-227) — export { StreamableHTTPServerTransport, EventStore } from '.../streamableHttp.js' becomes export { NodeStreamableHTTPServerTransport as StreamableHTTPServerTransport, EventStore } from '@modelcontextprotocol/server' with the same TS2305 — and in rewriteMockCall / rewriteDynamicImports (mockPaths.ts), though those cases are even rarer.

Impact

Nit severity: only one IMPORT_MAP entry has symbolTargetOverrides, importing the transport and EventStore/StreamId from streamableHttp.js in a single declaration with at least one alias is uncommon, and the resulting TS2305 is immediate and points directly at the offending specifier. The codemod is also documented as 80-90% coverage. But it's an internal inconsistency in newly-added logic — the per-symbol routing was clearly intended (it's implemented for the common case three lines below), and this branch silently produces broken output with no diagnostic.

Step-by-step proof

Input: import { StreamableHTTPServerTransport as Transport, EventStore as Store } from '@modelcontextprotocol/sdk/server/streamableHttp.js';

• importPaths.ts:106 — namedImports = [⟨StreamableHTTPServerTransport as Transport⟩, ⟨EventStore as Store⟩]; .some(n => n.getAliasNode()) → true.
• :107 — defaultImport=undefined, namespaceImport=undefined, hasAlias=true → enter branch.
• :109 — mapping.symbolTargetOverrides truthy, !namespaceImport && !defaultImport → enter.
• :110 — namedImports.every(...): 'StreamableHTTPServerTransport' in overrides ✓, 'EventStore' in overrides ✗ → false.
• :111-113 — skipped; effectiveTarget = '@modelcontextprotocol/server'.
• :115 — imp.setModuleSpecifier('@modelcontextprotocol/server').
• :117-121 — n.getName()='StreamableHTTPServerTransport' → n.setName('NodeStreamableHTTPServerTransport'); n.getName()='EventStore' → no rename.
• :123 — namespaceImport falsy → no warning.
• :134-135 — changesCount++; continue.

Final: import { NodeStreamableHTTPServerTransport as Transport, EventStore as Store } from '@modelcontextprotocol/server'; — NodeStreamableHTTPServerTransport is exported from @modelcontextprotocol/node, not /server. diagnostics = [].

Fix

Either drop the hasAlias short-circuit and let aliased named-only imports fall through to the per-symbol addPending loop (extending PendingImport to carry an alias?: string so addOrMergeImport can pass { name, alias } specifiers — ts-morph's insertImportDeclaration already accepts that shape), or keep the branch but split the declaration in place when symbolTargetOverrides && !allOverridden: move the overridden specifiers to a new import for the override target and leave the rest on targetPackage. At minimum, push a warning(filePath, line, ...) for this case so it's not silent.

What the bug is

collectFactorySymbols (mockPaths.ts:121-129) does:

factoryArg.forEachDescendant(node => {
    if (Node.isPropertyAssignment(node) || Node.isShorthandPropertyAssignment(node)) {
        symbols.push(node.getName());
    }
});

forEachDescendant visits every PropertyAssignment/ShorthandPropertyAssignment at any nesting depth inside the factory argument — not just the top-level keys of the object the factory returns. The result feeds the symbolTargetOverrides heuristic at line 105: factorySymbols.every(s => s in resolved.symbolTargetOverrides!). That check is meant to ask "is every export this factory mocks one that moved to the override package?" — but because nested object-literal keys (instance methods, config objects, etc.) leak into the list, the .every() is false whenever the factory contains any nested object literal.

Code path that triggers it

The canonical vitest/jest pattern for mocking a class is:

vi.mock('@modelcontextprotocol/sdk/server/streamableHttp.js', () => ({
    StreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({
        handleRequest: vi.fn(),
        sessionId: 'test-session'
    }))
}));

Walking through rewriteMockCall:

1. resolveTarget(...) → { target: '@modelcontextprotocol/server', renamedSymbols: { StreamableHTTPServerTransport: 'NodeStreamableHTTPServerTransport' }, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }.
2. Line 103-104: collectFactorySymbols(args[1]) walks the arrow function. forEachDescendant visits the outer StreamableHTTPServerTransport: ... assignment and the inner handleRequest: vi.fn() and sessionId: 'test-session' assignments → returns ['StreamableHTTPServerTransport', 'handleRequest', 'sessionId'].
3. Line 105: ['StreamableHTTPServerTransport', 'handleRequest', 'sessionId'].every(s => s in { StreamableHTTPServerTransport: '@modelcontextprotocol/node' }) → 'handleRequest' in {...} is false → allOverridden = false.
4. effectiveTarget stays at resolved.target = '@modelcontextprotocol/server'.
5. Line 110: firstArg.setLiteralValue('@modelcontextprotocol/server').
6. renameSymbolsInFactory then renames the top-level key to NodeStreamableHTTPServerTransport.

Output:

vi.mock('@modelcontextprotocol/server', () => ({
    NodeStreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({ handleRequest: vi.fn(), sessionId: 'test-session' }))
}));

But the migrated production code now does import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node' (per importPathsTransform's symbolTargetOverrides handling). The mock intercepts @modelcontextprotocol/server, not @modelcontextprotocol/node, so vitest/jest loads the real transport — the test silently exercises real I/O (or throws on construction) with no codemod diagnostic explaining why.

Why existing code doesn't prevent it

The existing test 'renames symbols in vi.doMock factory for streamableHttp' (mockPaths.test.ts:31-41) uses a flat factory () => ({ StreamableHTTPServerTransport: mockTransport }) with no nested properties, so collectFactorySymbols returns exactly ['StreamableHTTPServerTransport'], allOverridden is true, and /node is correctly chosen. Nothing exercises a nested .mockImplementation(() => ({...})) body, even though that's the dominant real-world shape for mocking a class constructor.

The same forEachDescendant depth issue exists in renameSymbolsInFactory (lines 131-153): a nested { McpError: someValue } inside a .mockImplementation body — i.e., an instance property on the mock, not a module export — would be renamed to ProtocolError, silently changing the mock's instance shape. That collision is less likely than the handleRequest case but is the same root cause.

Impact

When triggered, the user's migrated test file mocks the wrong module: the mock factory provides NodeStreamableHTTPServerTransport on @modelcontextprotocol/server (which doesn't export it), while the code under test imports it from @modelcontextprotocol/node and gets the real class. The failure surfaces as a test failure the user has to debug manually, with no codemod diagnostic — exactly the silent-test-behavior-change a migration tool should avoid. Nit severity because: only one symbolTargetOverrides entry exists today, it only affects test files, and the user's own test suite will catch the broken mock (loudly, if not informatively). But vi.fn().mockImplementation(() => ({...methods})) is the most common shape for class mocks, so the heuristic is broken for the majority of real inputs it was added to handle.

Step-by-step proof

Input:

vi.mock('@modelcontextprotocol/sdk/server/streamableHttp.js', () => ({
    StreamableHTTPServerTransport: vi.fn().mockImplementation(() => ({ handleRequest: vi.fn() }))
}));

• mockPaths.ts:80 — specifier = '@modelcontextprotocol/sdk/server/streamableHttp.js'; isSdkSpecifier → true.
• mockPaths.ts:82 — resolveTarget returns { target: '@modelcontextprotocol/server', renamedSymbols: {...}, symbolTargetOverrides: { StreamableHTTPServerTransport: '@modelcontextprotocol/node' } }.
• mockPaths.ts:102-103 — resolved.symbolTargetOverrides truthy, args.length === 2 → call collectFactorySymbols(args[1]).
• mockPaths.ts:123 — forEachDescendant on the arrow function visits, in order: the outer ObjectLiteralExpression's PropertyAssignment (StreamableHTTPServerTransport: ...) → push 'StreamableHTTPServerTransport'; then descends into vi.fn().mockImplementation(() => ({...})), reaches the inner ObjectLiteralExpression's PropertyAssignment (handleRequest: vi.fn()) → push 'handleRequest'. Returns ['StreamableHTTPServerTransport', 'handleRequest'].
• mockPaths.ts:105 — .every(s => s in { StreamableHTTPServerTransport: '@modelcontextprotocol/node' }): 'StreamableHTTPServerTransport' ✓, 'handleRequest' ✗ → false.
• mockPaths.ts:106 — skipped; effectiveTarget remains '@modelcontextprotocol/server'.
• mockPaths.ts:110 — specifier rewritten to '@modelcontextprotocol/server'.
• mockPaths.ts:115 — renameSymbolsInFactory renames the outer key to NodeStreamableHTTPServerTransport (and leaves handleRequest alone since it's not in allRenames).

Final: vi.mock('@modelcontextprotocol/server', () => ({ NodeStreamableHTTPServerTransport: ... })) — wrong target package.

Fix

Restrict collection to the top-level properties of the factory's returned object literal. Locate the returned expression (arrow body if a parenthesized object, or the ReturnStatement argument if a block body), guard with Node.isObjectLiteralExpression, and iterate .getProperties() filtered to PropertyAssignment | ShorthandPropertyAssignment:

function collectFactorySymbols(factoryArg: Node): string[] {
    let returned: Node | undefined;
    if (Node.isArrowFunction(factoryArg) || Node.isFunctionExpression(factoryArg)) {
        const body = factoryArg.getBody();
        returned = Node.isBlock(body)
            ? body.getStatements().find(Node.isReturnStatement)?.getExpression()
            : body;
        if (returned && Node.isParenthesizedExpression(returned)) returned = returned.getExpression();
    }
    if (!returned || !Node.isObjectLiteralExpression(returned)) return [];
    return returned
        .getProperties()
        .filter(p => Node.isPropertyAssignment(p) || Node.isShorthandPropertyAssignment(p))
        .map(p => (p as any).getName());
}

Apply the same top-level-only restriction to renameSymbolsInFactory (or have it operate on the same returned.getProperties() list) so nested instance-property keys aren't renamed. Add a test with vi.fn().mockImplementation(() => ({ handleRequest: vi.fn() })) asserting the specifier becomes '@modelcontextprotocol/node'.